API review

Proposer: Blaise

Present at review:

• Kevin
• Tully
• Jeremy
• James

Scope

C++ API. Should be fully documented in doxygen (if it has updated itself).

ROS API. Not properly documented yet:

• Updater has a parameter and publishes to /diagnostics.

Question / concerns / comments

Enter your thoughts on the API and any questions / concerns you have here. Please sign your name. Anything you want to address in the API review should be marked down here before the start of the meeting.

Blaise

• Should the node handle in the Updater constructor default to "~"?

• Should the updater and the DiagnosticTaskVector be split into separate files?

• Would be nice to resurrect ~Updater, but not currently possible.

• split_run could take just one argument. The summary could be merged into another location, and then copied back before publishing. This would make the API cleaner. If I do that then there is no longer any need to have a ComposableDiagnosticTask. Any tasks could be merged.

Kevin

• Should "node starting up" be an error?
• Tabs and indenting are completely messed up all over

• StatusWrapper should warn somehow when called with bad input? We've seen bad input with it before

• Looking at the prosilica node, it looks like Patrick didn't use the FrequencyStatus. Why not? Do we need a tutorial for this? He also used the deprecated stuff, even though he just added it.

• The deprecated stuff should be removed ASAP
• Needs HW ID field support. This should be part of every driver (#3221)

Self Test

• Should we switch to ros::Time instead of boost::get_system_time
• Why does it reset the id_ every time the test is called?
• How will the single threaded version work?
• Indents also bad

Meeting agenda

To be filled out by proposer based on comments gathered during API review period

Notes

• Why this package?
  • One thing to help fill out diagnostics
  • Periodic dissemination of information
  • Common diagnostics
• Node Starting up should be info and not an error.
  • BG: Done.
• Need good tutorials on non-deprecated API.
  • You will now find example.cpp that gives usage examples of most of the API.
• HW ID Support:
  • The updater should force people to set hardware id or WARN. Things without hardware IDs can set "None"
    • BG: Added a warning if all the diagnostics are OK but the hardware id is not set.
• Passed in nodehandle can become optional and default to "~"

  • Waiting to see what Wim concludes in the name harmonization process to decide which optional NodeHandles to pass in here.

Edit conflict - other version:

• BG: After discussion with Jeremy, just took the NodeHandle parameter out for now. We can add it as a non-API-breaking change later if a use case springs up. Jeremy says that the way in which we might want to push /diagnostics into a namespace is currently under discussion so it doesn't make sense to settle here yet.

Edit conflict - your version:

• BG: After discussion with Jeremy, just took the NodeHandle parameter out for now. We can add it as a non-API-breaking change later if there is a use case.

End of edit conflict

• Check how frequency parameter can be changed. Make it check every time update is called.
  • BG: Done.
• Possibly pull out common dependencies of diagnostic_updater and self_test.
  • Probably not since this requires touching a lot of code
    • BG: No action item here.
• Would be nice to send message at destruction, but ROS is already down at this point.
  • At the moment when diagnostics stop being broadcast, we get "stale" messages
  • Indicating a "clean shutdown" would be nice
  • Ticket this as a feature request, and talk to Josh about how to make this possible with ROS
    • Maybe request shutdown_callback from ROS
      • BG: Ticketed, no further action item for now.

• Would be nice to make it so all DiagnosticTasks are Composable

  • BG: Done.
• Action Item:
  • Document intended API usage.
  • API Review could benefit from an example
• This can't be run in the real-time loop. This is not something we should support. People doing diagnostics in realtime are responsible for calling update outside of the realtime loop.
  • BG: No action item.

Conclusion

Package status change mark change manifest)

• Blaise will change code/API from notes as appropriate

• Blaise will tie examples to some extra documentation on intended usage

• Reviewers will check off at that point if appropriate